Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up some duplicated code in cmd/ #3648

Merged
merged 6 commits into from
Jul 17, 2024
Merged

Conversation

hawkowl
Copy link
Collaborator

@hawkowl hawkowl commented Jun 26, 2024

Which issue this PR addresses:

Part of issues.redhat.com/browse/ARO-4895 (breaking out #3571)

What this PR does / why we need it:

Reduces some duplication in cmd/ that will get repeated in MIMO

Test plan for issue:

Unit tests, E2E

Is there any documentation that needs to be updated for this PR?

N/A

How do you know this will function as expected in production?

E2E

@hawkowl hawkowl added size-small Size small ready-for-review go Pull requests that update Go code skippy pull requests raised by member of Team Skippy labels Jun 26, 2024
@hawkowl
Copy link
Collaborator Author

hawkowl commented Jun 26, 2024

/azp run ci

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@hawkowl
Copy link
Collaborator Author

hawkowl commented Jun 26, 2024

/azp run e2e

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -0,0 +1,71 @@
package service
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make it more specific?
service is too general although it only concerns about cosmosdb.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Ayato. Maybe splitting them into util/database and util/encryption would help.

What do you think?

"github.com/Azure/ARO-RP/pkg/env"
)

func DBName(isLocalDevelopmentMode bool) (string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would make more sense to me if env package has this method (or core interface?).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with this one

@hawkowl
Copy link
Collaborator Author

hawkowl commented Jun 27, 2024

/azp run e2e

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@hawkowl hawkowl force-pushed the hawkowl/util-service-cleanup branch from e466bc8 to f64c758 Compare June 28, 2024 02:35
@hawkowl
Copy link
Collaborator Author

hawkowl commented Jun 28, 2024

/azp run ci, e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Collaborator

@mociarain mociarain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small suggestion but this is a great change imo

@@ -0,0 +1,71 @@
package service
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Ayato. Maybe splitting them into util/database and util/encryption would help.

What do you think?

@github-actions github-actions bot added the needs-rebase branch needs a rebase label Jul 12, 2024
Copy link

Please rebase pull request.

@hawkowl hawkowl force-pushed the hawkowl/util-service-cleanup branch from f64c758 to f408e3a Compare July 17, 2024 02:05
@github-actions github-actions bot removed the needs-rebase branch needs a rebase label Jul 17, 2024
@hawkowl
Copy link
Collaborator Author

hawkowl commented Jul 17, 2024

/azp run ci, e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@hawkowl
Copy link
Collaborator Author

hawkowl commented Jul 17, 2024

/azp run e2e

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@jaitaiwan jaitaiwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jaitaiwan jaitaiwan merged commit 81f22cb into master Jul 17, 2024
20 of 21 checks passed
@mociarain mociarain deleted the hawkowl/util-service-cleanup branch July 17, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code size-small Size small skippy pull requests raised by member of Team Skippy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants